Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

services/horizon/internal: Improve horizon history reaper #5331

Merged
merged 21 commits into from
Jun 24, 2024
Merged

services/horizon/internal: Improve horizon history reaper #5331

merged 21 commits into from
Jun 24, 2024

Conversation

tamirms
Copy link
Contributor

@tamirms tamirms commented Jun 3, 2024 •
edited
Loading

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    services/friendbot, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated any docs (developer docs, .md
    files, etc... affected by this change). Take a look in the docs folder for a given service,
    like this one.

Release planning

  • I've updated the relevant CHANGELOG (here for Horizon) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    semver, or if it's mainly a patch change. The PR is targeted at the next
    release branch if it's not a patch change.

What

Close #5339, #5338

This PR implements the following improvements to the horizon history reaper:

  • Add metrics for how many rows we delete during a reaping invocation and the duration of the reaping invocation
  • Ensure reaping is not run on multiple horizon nodes at the same time
  • Ensure reaping is only run on ingesting nodes (otherwise reaping will not work on horizon architectures where requests are served using read only replica dbs)
  • Remove statement timeout on reaping db connection to allow long running reaping DB statements to finish
  • Improve reaping so it can skip gaps
  • Reap history in ascending order so we can minimize gaps in case a reap invocation only partially succeeds

Known limitations

[N/A]

@tamirms tamirms changed the title services/horizon/internal: Add metrics to reaper services/horizon/internal: Improve horizon history reaper Jun 14, 2024
@tamirms tamirms marked this pull request as ready for review June 14, 2024 18:57
@tamirms tamirms requested a review from a team June 14, 2024 19:14
sreuland
sreuland approved these changes Jun 21, 2024
View reviewed changes
Copy link
Contributor

@sreuland sreuland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left minor comments for consideration, your call, nice work!

@tamirms
Copy link
Contributor Author

tamirms commented Jun 24, 2024

@sreuland @urvisavla could you take another look at the PR? I believe I have addressed all code review feedback. thanks!

@sreuland
Copy link
Contributor

@sreuland @urvisavla could you take another look at the PR? I believe I have addressed all code review feedback. thanks!

yes, lgtm, approved.

@tamirms tamirms merged commit b5c5b62 into stellar:master Jun 24, 2024
23 checks passed
@tamirms tamirms deleted the reap-metrics branch June 24, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@urvisavla urvisavla urvisavla left review comments

@sreuland sreuland sreuland approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Horizon reaper can run on multiple horizon nodes at the same time
3 participants
@tamirms @sreuland @urvisavla